-
Notifications
You must be signed in to change notification settings - Fork 2.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add DACs to support PID/VID verfification #14845
Add DACs to support PID/VID verfification #14845
Conversation
We are moving to have the certificate verification check the VID and PID between the basic cluster and the DAC/CD. Right now, none of the examples pass prorperly because the VIDs and PIDs do not match. In order to facilitate development while platforms are developing their own DeviceAttestationCredentialsProvider, we have provided a new set of development certs that can be used for development only. This new scheme is backed by the test PAA in attestation/test/. This was done to reduce the number of changes required to the controllers, which already contain this PAA in their trusted certs. The PAI has been changed to omit the PID. This means the we can use a common PAI cert for all products. The vendor ID for the PAI is 0xFFF1, which a known test vendor for Matter. The DACs below are signed by the new PAI and include certs and keys for PIDs 0x8000-0x801F.
Test: Can commission linux lighting app using pid 0x8000
This new CD will veryify against all products with VID 0xFFF1 and PIDs in the range of 0x8000-0x8063. Test: Verified on linux lighitng app by forcing app and controller to use pid 0x8001
Please see documentation in docs/examples.
…chip#14551)" (project-chip#14795)" This reverts commit ad28f32.
PR #14845: Size comparison from d5d9dc7 to 50dcf67 Increases above 0.2%:
Increases (19 builds for cyw30739, efr32, esp32, k32w, linux, mbed, qpg, telink)
Decreases (4 builds for esp32, linux, telink)
Full report (21 builds for cyw30739, efr32, esp32, k32w, linux, mbed, qpg, telink)
|
src/app/clusters/operational-credentials-server/operational-credentials-server.cpp
Outdated
Show resolved
Hide resolved
Might be a good idea to add the script that was used to generate all these certificates? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have to admit I did not verify that all the byte arrays and certificates are correct in any sense....
src/app/clusters/operational-credentials-server/operational-credentials-server.cpp
Outdated
Show resolved
Hide resolved
Co-authored-by: Tennessee Carmel-Veilleux <[email protected]>
Co-authored-by: Tennessee Carmel-Veilleux <[email protected]>
Co-authored-by: Evgeny Margolis <[email protected]>
everybody gets a bracket!
Do we currently have a tool to generate the certificates for a given "parameters" set ? |
I will add the script that was used to generate the certs in a new PR. It needs a bit of cleanup . |
PR #14845: Size comparison from 6f9fa8e to 4710a0c Increases above 0.2%:
Increases (27 builds for cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, qpg, telink)
Decreases (4 builds for esp32, linux, telink)
Full report (31 builds for cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, qpg, telink)
|
PR #14845: Size comparison from 52d0b42 to 689bcfa Increases above 0.2%:
Increases (27 builds for cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, qpg, telink)
Decreases (5 builds for esp32, linux, qpg, telink)
Full report (31 builds for cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, qpg, telink)
|
PR #14845: Size comparison from 52d0b42 to 120f9d6 Increases above 0.2%:
Increases (30 builds for cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, p6, qpg, telink)
Decreases (5 builds for esp32, linux, qpg, telink)
Full report (34 builds for cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, p6, qpg, telink)
|
/rebase |
I think this will appease the spell checker.
7df431a
to
1c8196e
Compare
PR #14845: Size comparison from 45b2690 to 1c8196e Increases above 0.2%:
Increases (16 builds for cyw30739, efr32, k32w, linux, p6, qpg, telink)
Decreases (3 builds for linux, telink)
Full report (17 builds for cyw30739, efr32, k32w, linux, p6, qpg, telink)
|
PR #14845: Size comparison from 45b2690 to dadb108 Increases above 0.2%:
Increases (39 builds for cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, p6, qpg, telink)
Decreases (13 builds for esp32, linux, telink)
Full report (43 builds for cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, p6, qpg, telink)
|
Per request in project-chip#14845, issue project-chip#14758
) * Consolidate dac verification parameters into struct. Per request in project-chip#14845, issue project-chip#14758 * make struct not use references. * Fix up some naming.
Problem
#14551 introduced code to check that the VID and PID supplied by devices from the basic information cluster match the VID and PID specified in the attestation certificate.
Unfortunately, for almost all platforms, it didn't. The DAC being supplied was specific to VID 0xFFF1 and PID 0x8000 and almost none of the platform examples were using this.
As it is nice to be able to at least differentiate between examples, we have opted to allow examples to select a PID from withing a defined range and update the certificates to support this. All example code now uses VID 0xFFF1 and PID in the range of 0x8000-0x801F (which can be trivially updated to allow additional PIDs up to 0x8063).
Reviewers: Because there were a lot of cert files added and because there were a lot of fairly minor PID changes, it may be easier to review the individual commits in the PR. #14551 was previously approved and was reverted in order to wait for this PR to land.
Change overview
Testing
Tested by CI, unit tests